Improve Controller Script Error Reporting #2588
Conversation
|
I don't know if this should go into 2.3 but IMHO the current behaviour is confusing and a major annoyance when writing controller mappings. |
|
This overlaps with #1795 (comment) Which we'll release with 2.4 and we'll merge as soon as the 2.3 branch is cut. Please have a look at my branch and rebase there if you think this is still relevant. |
|
Nice! I think we should merge this PR for 2.3 and just replace it with your code in 2.4. It will be at least half a year between the 2.3 and 2.4 releases and I think controller mapping creators would really appreciate getting notified where exactly the syntax error is. |
|
Solving merge conflicts is not exactly my favorite activity, but someone gotta do it eventually. I'll do my best. |
Previously the backtrace was only added to the error details if the `--controllerDebug` flag was passed to Mixxx. Since the error details are only displayed if the user explicitly uncollapsed the "Details" expander and non-technical users won't be able to make use of the engine's error message and line number anyway, we can just show it unconditionally. This might even result in more helpful bug reports.
Holzhaus
force-pushed
the
script-error-reporting
branch
from
a814a36
to
0e29507
Compare
|
Ok, this is now in a good state IMHO. This should vastly improve DX for controller mapping developers. Please review. |
|
Anything else to do here? |
There was a problem hiding this comment.
When I save a script with a syntax error, then fix the error and save again, the script is not reloaded until I restart Mixxx. I see this in the log:
Critical [Controller]: DEBUG ASSERT: "m_pEngine != nullptr" in function QScriptValue ControllerEngine::wrapFunctionCode(const QString &, int) at /home/be/sw/mixxx/src/controllers/controllerengine.cpp:114
13698d7
to
1a15898
Compare
Fixed. The problem was that not all errors were handled equally. We now also catch errors when calling the controller's |
If a controller script contains syntax errors or cannot be opened (e.g. due to permission errors), we don't show a popup when trying to initalize it. The user will often only notice it when pressing a button when Mixxx fails to invoke
MyController.someFunction, not instantly when loading the script. This could even happen in the middle of a set, e.g. when using something like a Korg nanoKONTROL for FX only. This behaviour also makes it unnecessarily hard for mapping developers to debug issues, because the actual error message is not shown, just a meaningless error that was caused by the failed script load.This enables the error reporting on failed script load, and also uninitializes the script engine in that case. This means that a single error dialog will be shown when the script load fails, but subsequent button presses on the controller will do nothing and not open a popup every time. Auto-reloading the script when it changes (or the permissions are changed) will still work fine.
I also modified the error messages a bit. All error dialogs should now show the controller name (in case multiple controllers are connected). Script filenames and error messages from the OS or the script engine were moved into the "Details" section of the error messages, because in most cases they are not interesting to a regular user (failures when opening the file is a special case, but I moved the filename to the details part anyway for consistency).